@shmax Thanks for tackling this :-). I'm currently sick with some kind of nasty bug and can't think straight, but will try to review this properly in the next couple of days.

In the meantime, quick thoughts after glancing at it:

• Why are you depending on an unstable branch of php-enum? Is there a reason you can't use a stable release?
• Need to run composer style-fix; the travis builds are currently failing with code style errors.
• If you're changing the API anyway, then could $path be bundled in with ConstraintError? If we're making a new class for this, might as well make sure it has all the information.
• Same point as above re additional format-specific error information.
• I like the syntax for selecting an error to use - assume php-enum is using __callStatic()? Good way of doing it.
• Does php-enum allow extending the class and only defining a subset of new errors, and falling back to the parent definitions for missing stuff? That's really important - need it to avoid breaking people's translations whenever a new error is added. Your current approach requires the user to manage falling back to parent::getMessage() manually in some way, which seems like asking for trouble later when somebody doesn't think to do it.

[Ed: One other thought - any chance of more descriptive commit messages than 'ws'? That doesn't mean anything to me; have to click through to the commit to figure out what it is supposed to be.]

I'll squash in a bit... I couldn't get the style checker to work locally, so I've been letting travis figure it out. Consider it a wip for now. I'm out and about at the moment, but will resume in the morning.

Gotcha. What's the style-checker doing? It should run on everything but nightly / hhvm; if it's not working on your system then something rather strange is going on.

You can also get an executable phar here, which means you can run it without needing to go through composer - if you're having issues with the depended one, this may be a suitable temporary workaround.

It's something to do with line endings. I'm on Windows, and I've got git configured to sort them out when pushing and pulling, but apparently the style checker isn't as savvy--it just aborts right away, and doesn't get as far as doing any style analysis.

Why are you depending on an unstable branch of php-enum? Is there a reason you can't use a stable release?

No reason. Updated to latest release.

If you're changing the API anyway, then could $path be bundled in with ConstraintError? If we're making a new class for this, might as well make sure it has all the information.

Not sure what you're suggesting. $path is dynamic.

Same point as above re additional format-specific error information.

Not following.

That's really important - need it to avoid breaking people's translations whenever a new error is added.

I hadn't really gotten as far as deciding how to advise people wanting to localize. We could provide extra hooks, but really they could just do as @bighappyface suggests and post-process errors as they're caught. Definitely don't expect people to subclass ConstraintError, at least not in its current state. The only real difference is that now the meaning of $constraint is no longer ambiguous, and we have a clear, easy-to-read mapping of error strings to error codes in once place for reference.

As far as breaking their strings goes, well, I guess I don't follow that, either. I wouldn't expect adding new strings to break anything--they just wouldn't be localized until the client does something about them. We'll have to consider existing $constraint values to be part of the external API going forward, and be careful to update the release version accordingly if they ever do change.

I did realize that with the old error layout it would be difficult (though not impossible) for localizers to extract the arguments for their translation calls; everything was just mixed into the same error object. I've restructured constraint to be an array with keys for name and params, which should sidestep the difficulty. It will break bc for anyone that was doing anything programmatic with the old fields in the error arrays, unfortunately.

@bighappyface @erayd So what do you think?

@shmax I'm still sick, so I can't promise the world's best feedback, but I will take a decent look at this today and let you know.

No rush. What about you, @bighappyface? You seemed to have some opinions about this kind of thing...

While I am not opposed to the approach per se I am concerned by how loose we have become with this package and breaking changes.

Simply put, changing the major version of this package every third PR that comes in is frustrating to me and unfair to consumers. We have to do a better job at trying to make things better without redesigning them every time.

That being said, I ask that we start road-mapping releases and embrace the idea of breaking the API later while maintaining BC now. If we are to break the API then we should set goals for what needs to be changed and bundle them together as part of a plan and not a side-effect of any given PR. This will help us vet our changes and be far more prudent in determining what needs to happen, and how to get there, with due dilligence.

In this case that means not altering the method signature for addError nor it's return value array structure for the constraint element. Those changes can come as the second phase to this work, in another major release, where this first phase centralizes errors, establishes constants, and basically ends there.

To help on this effort I have linked this PR and the original issue to a new 5.2.0 project:

https://github.com/justinrainbow/json-schema/projects/1

I can see the wisdom in not altering the layout of the error object, but what's the harm in changing the addError signature? Do you consider that part of the public API?

@bighappyface I agree very strongly with everything you just said in those last two posts.

Not sure how much of the discussion around the v5 refactor you read (it started in the defaults PR and then moved to its own thing), but I was very much in favour of not breaking things, and decided to lose that fight, because I'm new here and the impression I was getting was that maintaining backwards compatibility was at best a minor concern, and you guys didn't care nearly as much about it as I wanted to - the understanding I took from those conversations was that, as far as this library is concerned, minimising the number of public methods on Validator was preferable to maintaining backwards compatibility. I don't regret the code that resulted (although I'm very pleased that the check / coerce aliases on Validator ended up back in the mix), or the valuable contributions others here (particularly @shmax) made to help bring it to that final state, but if maintaining backwards compatibility in the API has now become a priority, or my original understanding was incorrect, I am very much in favor of that.

To that end, would it be possible to also have as a project goal properly documenting the public API? I'm happy to assist with the parts I'm familiar with, but I don't know enough about the intent behind the various parts to do much of it justice. Currently, Validator seems to be the only 'official' public interface, but then vast swathes of the rest of the API seem to be considered unofficially part of what users are intended to interface with - there is no clear line here.

@shmax That's why I reckon the public API needs documenting (see above). It's not officially documented as part of the public API, but it seems to be treated like something an end user would interact with. Certainly getErrors is pretty obviously public by virtue of its intended use.

@shmax Noting @bighappyface's request, would you still like me to do a full code-review of this PR today, or would you prefer to make changes first?

Is there really that much to review? Here's the takeaway:

1. Moved error strings to ConstraintError, and did an accounting of all the possible values for $constraint (to eliminate ambiguity).
2. Changed the addError signature to take a ConstraintError (to enforce compliance with 1.)
3. Changed the format of the error object so that the constraint arguments can be easily extracted by someone wanting to pass them to their own localization mechanic.

That's pretty much it. @bighappyface disagrees with both 2. and 3., at least without some kind of long-term release plan, which is fine. I guess I'll just wait for further instruction from him.

@shmax When I do code reviews, I like to understand the full impact of every single changed line, and the full implications of any logic that changed as a result, whether intended or otherwise. This can take a while.

If all you're after is a review of the concept, then I'm very much in favour of tidying up the way errors are handled - I had a PR in the works myself following our discussion in the schema-validation PR, (although not yet at the point I'd be happy to submit it), however you beat me to it - you seem keen to tackle this task, and you doing it means I can keep focused on the schema validation (and that annoying recursion bug), so I'm all in favour of this one being yours :-).

Between my original review comment and what @bighappyface has said, there probably isn't too much more for me to say here unless you'd like a full code review, which I'm happy to do if you'd find it valuable, and was my original intention. If a code review isn't what you want, but you have specific questions or want feedback on something, please ask.

@shmax sorry to see you close and delete. I don't think you needed to go that far.

May I suggest a variation?

# before
$this->addError($path, 'There must be a minimum of ' . $schema->minItems . ' items in the array', 'minItems', array('minItems' => $schema->minItems));

# after
$this->addError($path, ConstraintError::MIN_ITEMS(), ConstraintError::MIN_ITEMS, array('minItems' => $schema->minItems));

This change would maintain the core of your contribution while also keeping the current method signature. I think we gain a great deal by centralizing and we can refactor addError in 6.0.0.

What do you think?

Regarding 5.0.0, you guys did a ton of work adding some necessary functionality that this package sorely needed to stay relevant. I felt your work merited a version, and your continued engagement and support merits much more. Now that a bit of a team is forming I think organizing will help us tremendously, and I hope to see this package continue to improve. Careful planning will focus work and help motivate efforts.

$this->addError($path, ConstraintError::MIN_ITEMS(), ConstraintError::MIN_ITEMS, array('minItems' => $schema->minItems));

That's still a different signature. The second argument to addError as it stands now is currently a string, whereas in your sample the second argument is an instance of ConstraintError (which extends Enum). I guess I don't see the rationale in considering something like addError to be an inviolable part of the public API--by that logic we should never touch, add, or remove any public methods on any of the constraint classes, and I think that might be taking things a bit too far.

I can go ahead and rearrange things a bit so that the original signature IS maintained (at least until 6.0) as you suggest, but we wouldn't have achieved the goal of helping the folks who want to localize without making the change to error output (we'd be closer, but they would be left in the position of having to write a lot of glue code to interpret each of the possible values for error.constraint).

Thanks for the encouragement. When do you see us doing a 6.0 release?

@bighappyface - Works for me :-).

@shmax - I agree that your work is valuable, and closing this PR isn't necessary - with a few tweaks it can be made to work while still maintaining backwards-compatibility. it doesn't change the signature because the current approach doesn't define a type, so we can accept any argument we like there. It doesn't have to be a string.

@everyone I do think we need to seriously consider how people will extend the error messages to their own subset though - we can certainly come up with a viable approach for that now, and without needing to change the existing method signatures. I don't think we need to actually create translations, but I do think we need to have a sensible approach that we can point to and tell users who want to translate to use, and it needs to be a strategy that allows people to only have a subset of the messages translated, and does not require them to catch errors. IMO that translation should not be making the user interpret the output; it makes much more sense to bake some kind of way to apply translations into the message resolver you're busy writing here. That could be as simple as passing it an array of translations, and just merging it into the active messages array.

Regarding which parts of the error API are considered public - this is why I think we really need some better documentation regarding what is public API, and what is not. There's no clear line right now, and it's extremely frustrating trying to work on this without knowing what's safe to step on without breaking things for users.

it doesn't change the signature because the current approach doesn't define a type, so we can accept any argument we like there. It doesn't have to be a string.

Okay, fine, it's true that the signature doesn't technically change, but if we're going on the assumption that people are currently making their own calls to addError--which I admit is possible--and passing in strings in good faith (or overriding the method and doing something of their own with the incoming string before passing it down to the base), then their code will still break.

As far as the specifics on localization go, let's not make any assumptions. There are many different schemes, and not all of them involve typing strings into the code (in fact I've never personally seen it done that way). More typically the translated strings come from a database, or generated resources that are maintained in a completely separate process. The changes I made in this PR did provide one crucial piece to help make them all possible, however, which was to separate the arguments that you pass along with the string from the rest of the information in the error array. As things stand now (in master), you would have to write a big crazy switch statement.

Anyway, I'll mull it over a bit more and see if I can come up with some kind of transitional compromise.

Okay, fine, it's true that the signature doesn't technically change, but if we're going on the assumption that people are currently making their own calls to addError--which I admit is possible--and passing in strings in good faith (or overriding the method and doing something of their own with the incoming string before passing it down to the base), then their code will still break.

It doesn't need to break if you add a simple typecheck to the method (not in the signature). If it's an instance of your constraint class, use the new behavior. Otherwise, stick with the old behavior (as far as the API is concerned), and handle it however seems appropriate.

I'm certainly curious to see what you come up with - overall I'm liking the direction your PR is heading in :-).

I know that the previous work done on errors was work that those folks needed. Simply put, we can't really know what folks are using, so I think we have to assume whatever is public is in use, and plan with this assumption in mind for future versions.

I put a tentative note in the 6.0.0 project for refactoring the method but we can layout whatever milestones we wish to satisfy another major version. I simply want a plan and concensus we can execute on.

Okay, open for business, again. I've changed the base to 6.0.0-dev, so go ahead and review.

@harej any comment?

Wonderful, thanks much.

I opted not to try to work the error-not-found message into ConstraintError--it implies that it's a string to be localized, when really the only time it should ever be encountered is in json-schema development. It just wasn't like the others, but I am now throwing a InvalidArgumentException.

As long as we're changing the addError API, I also swapped the order of $constraint and $path, because I couldn't find any cases of a $constraint not being passed in, but I couldn't make it non-optional and not come before the ones that are optional.

@bighappyface please review and merge if you're satisfied

I want this release badly for the centralization of error codes for translating..

Good grief, has this never been released?

@shmax It was merged three years ago, and currently resides in the master branch, but is not backportable for compatibility reasons (see #368).

@adjenks If you use the master branch, you can make use of this feature.